refactor: Address Copilot code review suggestions#21
Conversation
Apply optimizations and improvements from PR review: 1. Extract normalize_key() helper to eliminate code duplication - Consolidates case normalization logic used in two places - Improves maintainability 2. Optimize insert_nested() to avoid double allocation - Changed signature to accept &[String] instead of &[&str] - Eliminates intermediate Vec<&str> allocation - Reduces memory allocations for nested environment variables All tests pass. Performance improved for nested key processing.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the environment variable handling code by introducing a normalize_key helper method to eliminate duplicated logic for key normalization. The refactoring also simplifies the insert_nested function signature to accept Vec<String> instead of &[&str], removing unnecessary conversions.
- Extracted key normalization logic into a dedicated
normalize_keymethod - Refactored
insert_nestedto acceptVec<String>instead of&[&str] - Replaced duplicate conditional blocks with calls to
normalize_key
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn normalize_key(&self, key: &str) -> String { | ||
| if self.nested { | ||
| key.to_string() | ||
| } else { | ||
| key.to_lowercase() | ||
| } | ||
| } |
There was a problem hiding this comment.
The normalize_key method preserves case in nested mode, but lines 424 and 428-429 unconditionally lowercase keys even in nested mode. This creates an inconsistency: keys stored in flat_map via normalize_key (lines 380, 405) preserve case in nested mode, but when split and inserted into result (lines 424, 428-429), they are lowercased. This breaks the intended behavior documented in the method's comment. Either remove the lowercasing at lines 424 and 428-429 when in nested mode, or update normalize_key to always lowercase.
| let trimmed = key_check[prefix_str.len()..].trim_start_matches(&self.separator); | ||
| // Keep case for nested mode, lowercase for flat mode | ||
| let key_for_map = if self.nested { | ||
| trimmed.to_string() | ||
| } else { | ||
| trimmed.to_lowercase() | ||
| }; | ||
| let key_for_map = self.normalize_key(trimmed); |
There was a problem hiding this comment.
When case_sensitive is false, key_check is uppercased (line 375), so trimmed contains an uppercased string. Passing this to normalize_key in nested mode preserves the uppercase version, which likely breaks case-sensitive comparisons later. The original key should be used instead of key_check when extracting trimmed.
| @@ -395,12 +402,7 @@ impl Environment { | |||
|
|
|||
| if key_check.starts_with(&prefix_str) { | |||
| let trimmed = key_check[prefix_str.len()..].trim_start_matches(&self.separator); | |||
There was a problem hiding this comment.
When case_sensitive is false, key_check is uppercased (line 400), so trimmed contains an uppercased string. Passing this to normalize_key in nested mode preserves the uppercase version, which likely breaks case-sensitive comparisons later. The original override_key should be used instead of key_check when extracting trimmed.
| let trimmed = key_check[prefix_str.len()..].trim_start_matches(&self.separator); | |
| let trimmed = override_key[prefix_str.len()..].trim_start_matches(&self.separator); |
Apply optimizations and improvements from PR review:
Extract normalize_key() helper to eliminate code duplication
Optimize insert_nested() to avoid double allocation
All tests pass. Performance improved for nested key processing.